[ci] add coverage reporting workflow - #271
Conversation
![qoderai[bot]](https://avatars.githubusercontent.com/in/1396158?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR adds a well-structured coverage reporting pipeline. The overall design — YAML anchors for step reuse, a dedicated comment-pr-coverage job that only downloads artifacts (no code checkout), Python-based LCOV parsing with unit tests, and a gcov JSON isolation wrapper — is solid. A few issues need attention before merging:
Blockers
actions/upload-artifact@v6andactions/download-artifact@v6do not exist (latest isv4). The workflow will fail at runtime.
Security
- Runtime download of unsigned
jqandghbinaries with no checksum verification, executed in a step that holdsGH_TOKEN. Pre-install in the image or use theghbinary already present onubuntu-latestrunners. --privileged+ full host-root mount (/:/host_root/) is broader than needed; scoping the mount or separating the disk-cleanup step would reduce the blast radius.
Logic
- The
links/ workflow-run URL conditional in the PR comment script is broken: the workflow run link is appended unconditionally regardless of the branch taken (lines 255–263). DA:hit counts within a single LCOV record are overwritten instead of accumulated, which can under-report coverage.non_coverable(changed lines without LCOV data) could theoretically go negative; amax(0, ...)guard is advisable.
Observability
- If the coverage job fails before the upload step,
comment-pr-coveragesilently skips. A fallback "coverage run failed" comment would make it easier to notice failures on PRs.
🤖 Generated by Qoder
| run: | | ||
| set -x | ||
| ARCH=$(uname -m | sed 's/x86_64/amd64/;s/aarch64/arm64/') | ||
| curl -sLo /usr/local/bin/jq "https://github.com/jqlang/jq/releases/download/jq-1.8.1/jq-linux-${ARCH}" && chmod +x /usr/local/bin/jq |
There was a problem hiding this comment.
Downloading jq and gh binaries at runtime over plain HTTP/redirected HTTPS introduces a supply-chain risk: there's no checksum or signature verification. If the GitHub release URL is compromised or the redirect is MITM'd, the binary that runs with GH_TOKEN access could exfiltrate the token or delete arbitrary caches.
Consider either:
- Pinning with a
sha256check:echo "<expected-sha256> /usr/local/bin/jq" | sha256sum -c - Installing both tools once in the dev container image (they're already needed for CI maintenance work).
- Using the
ghCLI that ships with GitHub-hosted runners (it's pre-installed onubuntu-latestat/usr/bin/gh).
🤖 Generated by Qoder
| - *upload_coverage_artifacts_step | ||
|
|
||
| comment-pr-coverage: | ||
| if: ${{ always() && github.event_name == 'pull_request' && (needs.coverage-github.outputs.coverage-artifact-url != '' || needs.coverage-aliyun.outputs.coverage-artifact-url != '') }} |
There was a problem hiding this comment.
The comment-pr-coverage job fires when always() is true and at least one artifact URL is non-empty. However, both coverage-github and coverage-aliyun are mutually exclusive by design (only one will run per workflow trigger), so needs.coverage-github.result and needs.coverage-aliyun.result will be 'skipped' for the job that didn't run.
always() prevents the job from being skipped when its dependencies were skipped, but the artifact URL check already handles the empty-URL case. One edge case: if the coverage job itself fails before the upload step, the artifact URL will be empty and the comment job silently skips. It may be worth adding a fallback comment (e.g. "coverage run failed") so the PR isn't left without feedback.
🤖 Generated by Qoder
| const links = artifactUrl | ||
| ? `HTML report and raw LCOV: [coverage-report](${artifactUrl})` | ||
| : `Workflow run: [${context.runId}](${runUrl})`; | ||
| const body = [ |
There was a problem hiding this comment.
When artifactUrl is truthy, links is set to the artifact URL line, but Workflow run: [${context.runId}](${runUrl}) is then appended unconditionally as a separate element in body (line 263). This means the workflow run link is always included regardless of the links branch, which makes the conditional on line 255–257 misleading — the fallback case also ends up with both lines. Consider either removing the conditional and always including both, or removing the duplicate from body.
🤖 Generated by Qoder
| container: | ||
| image: ghcr.io/alibaba/tair-kvcache-kvcm-dev:2026_02_13_12_03_24230b1 | ||
| volumes: | ||
| - /:/host_root/ |
There was a problem hiding this comment.
Mounting the host root (/:/host_root/) with --privileged is necessary for the disk-space cleanup step, but it significantly widens the blast radius if any step in the job is compromised. Consider scoping the mount to only the directories actually cleaned (/usr/share/dotnet, /usr/local/lib/android, /opt/ghc) or moving the cleanup to a separate pre-job step that doesn't need it, so the main coverage steps run with reduced privileges.
🤖 Generated by Qoder
| elif line.startswith("DA:") and current_file: | ||
| fields = line[3:].split(",", 2) | ||
| if len(fields) >= 2: | ||
| current_lines[int(fields[0])] = max(int(fields[1]), 0) |
There was a problem hiding this comment.
The DA: accumulation logic (current_lines[int(fields[0])] = max(int(fields[1]), 0)) overwrites earlier hits for the same line number within a single record if the same line appears twice. The correct semantics is to accumulate within a record too (matching the cross-record merge done in flush_record). This could under-report hits for pathological but valid LCOV files.
current_lines[int(fields[0])] = current_lines.get(int(fields[0]), 0) + max(int(fields[1]), 0)🤖 Generated by Qoder
0a23cf0 to
a9647f7
Compare
a9647f7 to
654ec16
Compare
Coverage Summary
Diff base: HTML report and raw LCOV: coverage-report |
1 participant
Summary
coverageGitHub Actions workflow for Bazel LCOV collectiontools/coverage/run_coverage.shNotes
//kv_cache_manager/...and//integration_test/...with--config=debug --config=ci_fast--config=client; client coverage can be handled separatelyrebuildDiskCache=true;scheduled-cache-rebuilddispatches this rebuild nightly onmainruns-onorCOVERAGE_RUNS_ONissues: write; coverage jobs keep the default read/cache permissionsValidation
bash -n tools/coverage/run_coverage.sh tools/coverage/gcov_json_isolated.shPYTHONPATH=tools/coverage python3 -m unittest tools.coverage.coverage_report_test.github/workflows/coverage.ymland.github/workflows/scheduled-cache-rebuild.ymlgit diff --check origin/main...HEAD//kv_cache_manager/common/test:LoopThreadTest, including HTML generationaliyun-ecs-x64: https://github.com/alibaba/tair-kvcache/actions/runs/30899860220